fix: Implement migration adapter check for textual settings parameters. #4515
Conversation
prmukherj
requested review from
mkundu1,
seanpearsonuk,
hpohekar and
mayankansys
as code owners
|
The description! |
Co-authored-by: Sean Pearson <93727996+seanpearsonuk@users.noreply.github.com>
Done, Thank you |
|
@prmukherj Can you please add a test? |
yes, sure, will add one before merging. Thank you for mentioning it. |
@mkundu1, added the test mentioned in the fluent bug with a different case file. |
|
Reading the description, it’s hard to tell which parts describe the current behavior and which describe the changes introduced in this PR. This is a wider issue for us: our PR descriptions often don’t make it easy to reconstruct what changed and why. Ideally, we should aim for descriptions that make sense even years later when reading the commit history. At the moment, we’re falling short of that goal, and this PR is a good example of where we could do better. |
@seanpearsonuk, could you please have a look if it is better now? |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a type-checking issue in PyFluent where migration adapters were being bypassed due to premature TypeError exceptions. The change allows migration adapters to handle type conversions for textual settings parameters when they are available.
- Modified type-checking logic in
set_statemethod to check for migration adapter presence before raising TypeError - Added comprehensive test coverage for the migration adapter functionality with boolean-to-string conversion scenarios
- Updated changelog to document the fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ansys/fluent/core/solver/flobject.py | Modified set_state method to bypass type checking when migration adapter exists |
| tests/test_settings_api.py | Added test cases to verify migration adapter functionality with boolean-to-string conversions |
| doc/changelog.d/4515.fixed.md | Added changelog entry documenting the migration adapter fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if self._has_migration_adapter: | ||
| return self.base_set_state(state=state, **kwargs) |
There was a problem hiding this comment.
The _has_migration_adapter property is not defined or documented in the visible code. Consider adding a comment explaining what this property represents and how it determines if a migration adapter is available.
Copilot uses AI. Check for mistakes.
prmukherj
deleted the
fix/update_migration_adapter_for_textual_settings_params
branch
This change updates the type-checking logic to correctly handle cases where a migration adapter is present.
Previous Behavior
When setting a textual attribute through PyFluent, type checking was performed before delegating to the underlying Settings API.
If the provided value did not strictly match the expected type, PyFluent raised a TypeError, even in cases where a migration adapter existed to handle type conversion.
For example:
Here, the property accepts a string value, but a migration adapter is available that correctly converts a boolean (False) into the appropriate string value.
Previously, PyFluent would still raise a TypeError before this conversion could occur.
Current Behavior
If a migration adapter exists for a setting, PyFluent now bypasses type checking and directly forwards the input to the Settings API.
This allows the migration adapter to handle type conversions or other logic as intended.
Please refer to the test to see the detailed usage.
The change aligns PyFluent behavior with the Fluent-side fix (ref. bug 1293534).